-
Notifications
You must be signed in to change notification settings - Fork 984
Toolchain reinstalling fix 2571 #2573
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Toolchain reinstalling fix 2571 #2573
Conversation
Signed-off-by: Daniel Silverstone <dsilvers@digital-scurf.org>
When we check if an overridden toolchain has everything it needs, we sometimes decided to reinstall despite everything being present. This reworks the decision-making to take into account rewritten component names, and also to handle targets properly. Signed-off-by: Daniel Silverstone <dsilvers@digital-scurf.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a little hard to infer from the diff alone. What caused the repeated syncs?
|
The issue was twofold.
|
| manifest | ||
| } else { | ||
| // If we can't read the manifest we'd best try and install | ||
| return Ok(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will this remove the corrupt dist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it'll bubble the situation up a level which will then try to ensure the component set in question and report something along the lines of:
error: toolchain 'nightly-2020-11-24-x86_64-unknown-linux-gnu' does not support components
I think this is fair enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except that that should support components, and the reason it doesn't is that its corrupt, and we know its corrupt because we expected it to support components, and we returned Ok(true) rather than signalling the impossible situation...
I think this is good enough to do a release with; I don't think its good enough to consider done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually no, I got that entirely wrong - argh. Let me faff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think when I tested this, I must have accidentally reinstalled the test release rather than the version I was editing. I need to push a change to make that Ok(false) and then instead of being crap about it, it'll heal the installation.
This fixes #2571 by rewriting the check for "is this already installed"